PYTHON-5947 Add OpenTelemetry operation spans - #2991
Conversation
4a8db0c to
ba21bb8
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
c841c9f to
4e0c468
Compare
Give every public API call an operation span containing one command span per command sent to the server, per the OpenTelemetry driver specification. The span covers all retry attempts of one _retry_internal call, so retries appear as sibling command spans rather than being collapsed into one. killCursors and endSessions bypass the retry layer and start their spans at the call site instead, as does an unacknowledged client bulk write, which never reaches the command-span code that would fill in its namespace. Cursor-creating operations (find, aggregate, listCollections, listIndexes) are covered here, but only for the command that creates the cursor. Spans for caller-driven getMores come in a later change, as do transaction spans. _otel.py also takes over the specification's naming and attribute rules, so _telemetry.py deals only with span lifecycles and a specification change need not touch it. Namespace parsing moves to helpers_shared._split_namespace.
4e0c468 to
6638ae1
Compare
Lead with the condition rather than the bare adjective.
The vendored unified spec fixtures it pointed at arrive in a later change, so the note described tests absent from this one.
…d test Evergreen only runs on PRs whose base is a configured branch, so the upper PRs in this stack get no Evergreen coverage, and the otel tests only run in Evergreen's otel variant. Add a temporary GitHub Actions job that runs them on every branch in the stack, against a replica set so the transaction span tests are included. Remove the job before merging. Also drop test_operation_name_normalizes_enum_operation. All 22 vendored fixtures assert db.operation.name with literal values, so the _Op formatting regression it guards fails them loudly, and adding a test here only to delete it once the fixtures land is churn.
|
Semgrep found 2 GitHub Actions step uses a mutable tag or branch reference. Tags and branch names can be silently repointed by the action owner, enabling supply-chain attacks — as seen in the trivy-action and kics-github-action compromises. Pin the reference to a full 40-character commit SHA instead, e.g. 🛟 Help? Slack #semgrep-help or go/semgrep-help. Resolution Options:
|
…the end The mutable-action-tag scanners reported two findings in test_minimum, which this branch does not touch: inserting the otel job mid-file shifted those lines by 31 and the diff-based scan attributed them to this change. Appending the job instead leaves every pre-existing line where it was. Pin the job's own actions to commit hashes so the newly added lines do not trip the same rule. drivers-evergreen-tools has no pinned use elsewhere in this file, so v1.0.1 is spelled out; its commit is the same one master points at, so the topology input this job relies on is unchanged.
NoahStapp
left a comment
There was a problem hiding this comment.
Can you run a micro benchmark measuring performance impact both when OTel is enabled as well as disabled?
| :param is_run_command: If this is a runCommand operation, defaults to False. | ||
| :param is_aggregate_write: If this is a aggregate operation with a write, defaults to False. | ||
| :param operation_id: Stable operation id shared across retries, defaults to None | ||
| :param operation_telemetry: A caller-owned operation span outliving this call, |
There was a problem hiding this comment.
How can the operation span outlive this call and this method own a fresh span? Not clear what that means.
| dbname: str, | ||
| collection: Optional[str] = None, | ||
| ) -> _CommandCursor: | ||
| """Run a command-cursor read within its own operation span. |
There was a problem hiding this comment.
| """Run a command-cursor read within its own operation span. | |
| """Run a command cursor read within its own operation span. |
No hyphen between command and cursor.
| # One span covering every attempt. A caller needing it to outlive this | ||
| # object (a cursor) passes its own and keeps ownership. |
There was a problem hiding this comment.
self._owns_telemetry = True means that we create the span here, otherwise it's passed in because a cursor owns it? Comment doesn't make that clear.
| ``query_text_max_length`` is None as validated from user input; the options | ||
| a client holds have been through :func:`_resolve_tracing_options`, so both | ||
| fields are resolved. |
There was a problem hiding this comment.
Does this mean that query_text_max_length is always None? Not sure what this comment means.
| return f"{command_name} {dbname}" | ||
|
|
||
|
|
||
| # Some `_Op` values are the wire command name ("drop"/"create") rather than the |
There was a problem hiding this comment.
Why not rename _Op for consistency if it's entirely internal?
| # limitations under the License. | ||
|
|
||
| """Test OpenTelemetry command-span support.""" | ||
| """Test OpenTelemetry operation spans, excluding cursor getMores.""" |
There was a problem hiding this comment.
Are cursor getMores tested elsewhere?
| # spec's db.operation.name ("dropCollection"/"createCollection"). Translate here | ||
| # instead of renaming `_Op`: `_WRITES_WITH_CLUSTER_TIME` in operations.py matches | ||
| # these exact strings to pick which writes get afterClusterTime. | ||
| _OPERATION_NAME_OVERRIDES = { |
There was a problem hiding this comment.
I think we need "rename -> renameCollection" here too.
| # built inside it. Before the sensitive-command return below, since the | ||
| # operation span needs those attributes even when the command gets no span. | ||
| current_operation = _CURRENT_OPERATION_NAME.get() | ||
| if current_operation is not None: |
There was a problem hiding this comment.
Does this backfill run on each retry attempt too? If so, does it need to?
| # ___init__ did not run to completion (or at all). | ||
| return | ||
|
|
||
| self._end_operation_telemetry() |
There was a problem hiding this comment.
Shouldn't this come after the actual close, so if the close fails this also records the failure?
| except BaseException as exc: | ||
| if operation_telemetry is not None: | ||
| operation_telemetry.failed(exc) | ||
| raise | ||
| else: | ||
| if operation_telemetry is not None: | ||
| operation_telemetry.succeeded() |
There was a problem hiding this comment.
Can this not use the with telemetry or contextlib.nullcontext() pattern used elsewhere instead? Same question twice in mongo_client.py too.
PYTHON-5947
First of four PRs splitting #2964. Base is
otel; review in order.PYTHON-5947-otel-1-operationsPYTHON-5947-otel-2-transactionsPYTHON-5947-otel-3-unifiedPYTHON-5947-otel-4-getmoreChanges in this PR
Adds one span per public API call, containing one command span per command sent to the server, per the OpenTelemetry driver specification. Without it a retried operation's command spans share no parent, so a reader cannot tell which attempts belong to the same call.
killCursors,endSessions, and unacknowledged client bulk writes get operation spans too.find,aggregate,listCollections,listIndexes) are covered, but only for the command that creates the cursor. Caller-drivengetMores arrive in PR 4.0,False,""). This affected structured command logging as well as spans.Opt-in: no behavior change unless the
tracingclient option orOTEL_PYTHON_INSTRUMENTATION_MONGODB_ENABLEDis set. With tracing off there is no added allocation per command.Around 1,000 of the added lines are
just synchrooutput and another ~745 are tests.Test Plan
just lintclean.Checklist
Checklist for Author
Checklist for Reviewer